Add hash functions for group elements #5479

jamesnohilly · 2025-10-20T15:59:08Z

This PR adds hash functions for group elements (see #5340).

fingolfin · 2025-10-22T11:53:20Z

src/Groups/types.jl

 const SubPcGroupElem = BasicGAPGroupElem{SubPcGroup}

+function Base.hash(x::Union{PcGroupElem,SubPcGroupElem}, h::UInt)
+  return hash(letters(x), hash(parent(x), h))


Just been talking about this with @jamesnohilly -- he pointed out that hash(parent(x), h) currently just returns h but that he included this because it seems sensible, to which I generally agree. However, I've then been thinking: I believe we allow comparing a PcGroupElem to a SubPcGroupElem if both come from the same "full" group -- is that right, @ThomasBreuer ? If so, then one may have g == h being true, where one is a PcGroupElem and then other a SubPcGroupElem -- but they have different parent groups. This then of course would lead to a correctness issue if those two parents eventually do get a working hash method, assuming that hash method would produce different results...

So perhaps we should not take the parent into account, but rather full_group(parent(x)).

But perhaps the real issue is that we allow comparing a PcGroupElem with a SubPcGroupElem in the first place... for rings, we don't generally allow comparing elements with different parents (we have a bunch of open PRs for AA/Nemo to make that stricter, another stalled discussion... sigh)

Concerning comparisons of group elements with different parents:
We support this whenever the parents are "compatible", which roughly means that there are natural embeddings of the two parents into a common group.
For example, permutations of the same degree have compatible parents,
and PcGroupElems and SubPcGroupElems with same full group have compatible parents.

If we want to use parent information for the hash value, the full group of a PcGroupElem or SubPcGroupElem would logically be the right object.
Analogously, the degree (which is not known on the GAP side) could then be used for the hash of PermGroupElems.

gap/pkg/OscarInterface/gap/hash.gi

fingolfin · 2025-10-22T12:59:53Z

src/Groups/types.jl

+  b = UInt(GAPWrap.HashPermutation(GapObj(x)))
+  return xor(h, b)


The second argument to GAP's HashKeyBag is actually also a seed. So we could have something like this:

Suggested change

b = UInt(GAPWrap.HashPermutation(GapObj(x)))

return xor(h, b)

return UInt(GAPWrap.HashPermutation(GapObj(x), GapInt(h)))

and then adjust HashPermutation suitably.

And of course a coupld tests in test/Group/ would be nice -- just compute the hash of a couple permutations etc., and e.g. create (1,2) twice and make sure both copies have the same hash, and different from that of (). Or so

src/Groups/types.jl

lgoettgens · 2025-11-06T16:15:57Z

Just been talking about this with @jamesnohilly -- he pointed out that hash(parent(x), h) currently just returns h

This is not true. We changed hashing for GapObj to throw an error, cf https://github.com/oscar-system/GAP.jl/blob/a4faccc28634dc3c6156e90ead9cbaef4aafc70d/src/adapter.jl#L417

But since I still agree that the hashing in Oscar should make use of the composition of different hash methods, I would be in favor of removing the error there and instead returning h.

jamesnohilly · 2025-11-06T16:52:04Z

Just been talking about this with @jamesnohilly -- he pointed out that hash(parent(x), h) currently just returns h

For this specifically, I think the context was in relation to (sub)types of GAPGroup and GAPGroupElem, which currently is defined as Base.hash(x::GAPGroup, h::UInt) = h. However I have noticed a typo where I tried to hash an GapObj instead of the resulting group from a tuple, which might be causing some test errors.

However I agree this could be something that might cause further issues.

ThomasBreuer · 2025-11-06T17:08:02Z

Concerning error as the hash method for GapObjs:
The idea is that GAP does not give us meaningful hash values for GAP objects. Thus we should not accidentally call hash with GapObjs.
(Just this week I stumbled over a method in Oscar where this still happens, apparently this method is never used, otherwise we would see the error.)

hash methods for GAPGroups are of course fine, just they cannot delegate the work to their GapObjs.

lgoettgens · 2025-11-06T18:32:05Z

ah, yeah, I was confused by the similarity of the terms GapObj and GapGroup, and the error in CI. Thanks for spotting and fixing the error!

I think this is in a position now where it could be un-drafted to get a final round of reviews.

ThomasBreuer · 2025-11-07T09:13:21Z

src/Groups/types.jl


+function Base.hash(x::Union{PcGroupElem,SubPcGroupElem}, h::UInt)
+  G = full_group(parent(x))[1]
+  return hash(letters(x), hash(G, h))


If we think of groups with large relative orders then letters(x) can be expected to be a long list.
syllables(x) would usually be shorter.

Both letters and syllables create new objects. Ideally we would take the internally stored exponent vector; for that, a GAP function analogous to HashPermutation would be needed.

ThomasBreuer

Tests for hashing PcGroupElem, SubPcGroupElem, MatrixGroupElem are missing.

lgoettgens · 2025-11-16T10:24:41Z

test/Groups/Permutations.jl

+  c = perm(g, [1, 2, 3])
+
+  @test hash(c) == hash(one(g))
+  @test hash(a) != hash(one(h))


I am not a fan of these kinds of tests, as they can non-deterministically fail due to hash collisions

lgoettgens · 2025-11-16T10:25:52Z

src/Groups/types.jl

 end

+function Base.hash(x::MatrixGroupElem, h::UInt)
+  return hash(matrix(x), hash(base_ring(parent(x)), h))


Suggested change

return hash(matrix(x), hash(base_ring(parent(x)), h))

return hash(matrix(x), hash(parent(x), h))

IMO it is better to compose hash methods, ie just take the hash of the parent here, and then add a hash function for the parent that takes the base_ring into account

lgoettgens · 2025-11-16T10:27:00Z

src/Groups/types.jl

+function Base.hash(x::Union{PcGroupElem,SubPcGroupElem}, h::UInt)
+  G = full_group(parent(x))[1]
+  h = is_finite_order(x) ? hash(order(x), h) : h
+  h = hash(is_finite(G), hash(G, h))


Suggested change

h = hash(is_finite(G), hash(G, h))

h = hash(G, h)

IMO the is_finite(G) part should already be used in hashing G, so it would be redundant here. Changing this makes it easier to compose different hash methods

Add element hash methods

089fa0a

jamesnohilly added topic: groups release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Oct 20, 2025

fingolfin reviewed Oct 22, 2025

View reviewed changes

jamesnohilly added 2 commits October 22, 2025 13:18

Add GAP interface for Perm hash

9453b71

Add hash function for perm

d25a600

fingolfin reviewed Oct 22, 2025

View reviewed changes

gap/pkg/OscarInterface/gap/hash.gi Outdated Show resolved Hide resolved

fingolfin reviewed Oct 22, 2025

View reviewed changes

lgoettgens reviewed Oct 23, 2025

View reviewed changes

src/Groups/types.jl Outdated Show resolved Hide resolved

jamesnohilly added 5 commits November 5, 2025 16:15

Allow seed from Julia for hash

b13faaa

Remove hash for FPGroupElems

6a7e228

Add test for hashing permutations

f781cd3

Use full_group for PcGroupElems

a80e4f3

Fix tests

806b407

Fix typo

f5f74b9

ThomasBreuer reviewed Nov 7, 2025

View reviewed changes

jamesnohilly added 2 commits November 15, 2025 13:40

Add test for PcGroupElems

bbbfc66

Add test for MatrixGroupElem

6968981

jamesnohilly marked this pull request as ready for review November 15, 2025 14:09

lgoettgens reviewed Nov 16, 2025

View reviewed changes

		b = UInt(GAPWrap.HashPermutation(GapObj(x)))
		return xor(h, b)

	b = UInt(GAPWrap.HashPermutation(GapObj(x)))
	return xor(h, b)
	return UInt(GAPWrap.HashPermutation(GapObj(x), GapInt(h)))

	return hash(matrix(x), hash(base_ring(parent(x)), h))
	return hash(matrix(x), hash(parent(x), h))

Add hash functions for group elements #5479

Are you sure you want to change the base?

Add hash functions for group elements #5479

Conversation

jamesnohilly commented Oct 20, 2025

Uh oh!

fingolfin Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

ThomasBreuer Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fingolfin Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgoettgens commented Nov 6, 2025

Uh oh!

jamesnohilly commented Nov 6, 2025

Uh oh!

ThomasBreuer commented Nov 6, 2025

Uh oh!

lgoettgens commented Nov 6, 2025

Uh oh!

ThomasBreuer Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

ThomasBreuer left a comment

Choose a reason for hiding this comment

Uh oh!

lgoettgens Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

lgoettgens Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

lgoettgens Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants